Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FF-2525 Add Assignment Details to logger #106

Merged

Conversation

greghuels
Copy link
Contributor

Implements FF-2465

Motivation and Context

Most people who are using the assignment logger are doing it to gain more insight on the assignments. It makes sense to include the "details" as well so that they have access to all the information available.

Description

Added "details" to assignment, cleaned up imports, and renamed interfaces to follow convention.

@greghuels greghuels force-pushed the greg/FF-2525/assignment-logger branch from 274704a to ba69a83 Compare June 27, 2024 20:58
@greghuels greghuels force-pushed the greg/FF-2525/assignment-logger branch 2 times, most recently from 15287af to 992e23c Compare June 27, 2024 21:14
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thinking!

@@ -622,6 +622,7 @@ export default class EppoClient {
sdkLanguage: 'javascript',
sdkLibVersion: LIB_VERSION,
},
details: result.flagEvaluationDetails,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵️‍♂️ Spotted the actual change! 😛


import EppoClient, {
AssignmentDetails,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing these were unused imports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

@@ -26,7 +26,7 @@ export interface AllocationEvaluation {
orderPosition: number;
}

export interface FlagEvaluationDetails {
export interface IFlagEvaluationDetails {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Yes this is our naming convention for interfaces

@@ -11,6 +12,22 @@ describe('IAssignmentEvent', () => {
timestamp: new Date().toISOString(),
subjectAttributes: { age: 25, country: 'USA' },
holdoutKey: 'holdout_key_123',
details: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this test was not obvious to me until I ran it myself. It will fail if any required attribute in IAssignmentEvent is missing but will pass if attributes not specified in IAssignmentEvent are included in the event object (the latter behavior is described by the test "it" statement)

Just a suggestion (feel free to take it or leave it): if it's not too much trouble, you could add a test that checks for a thrown exception if details (a required attribute in the event object) is missing

@greghuels greghuels force-pushed the greg/FF-2525/assignment-logger branch from 992e23c to 7793a4c Compare June 28, 2024 09:26
@greghuels greghuels merged commit f340949 into greg/FF-2465/evaluation-reasons Jun 28, 2024
2 checks passed
@greghuels greghuels deleted the greg/FF-2525/assignment-logger branch June 28, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants